Conversation
Codecov Report
@@ Coverage Diff @@
## development #632 +/- ##
===============================================
- Coverage 84.95% 84.81% -0.14%
===============================================
Files 335 335
Lines 24798 24798
Branches 11249 11262 +13
===============================================
- Hits 21066 21033 -33
Misses 3729 3729
- Partials 3 36 +33
|
|
Could you test it with Intel 19? I could send this one-liner to the original report, but it may take some time to get feedback. |
fuchsto
left a comment
There was a problem hiding this comment.
The subclassing was not unnecessary, it indicates the type category.
This is why public inheritance was used. The formal reading is "RegionSpec IS_A Dimensional<uint8_t, D>".
The compiler error is right, though, the fix is to add missing delegate constructor calls in RegionSpec.
|
RegionSpec is not really "a" Dimensional. The member RegionCoords is Dimensional. But RegionSpec only combines RegionCoords with some other properties. |
|
Well, the question whether "A is a B" ( SizeT volume(const Dimensional & d) { return std::accumulate( ... , std::multiplies); }
RegionSpec<...> rs(...);
auto region_volume = volume(rs);I don't see why something like that should not be okay, so |
|
What is the way forward here? I don't remember this was discussed at the F2F but it keep biting... |
|
i still think that no inheritance should be used. I don't use the data structure of the base class or any methods. |
|
Can the |
|
Not really, because RegionSpec combines RegionCoords (Dimensional), the extent and some other useful things. In an older version i didn't used RegionCoords, but instead Dimensional to store the coordinates. Later i devided them. And at this point i missed to delete the inheritance. |
What's the reason to not use |
|
Because i needed RegionCoords separately for other components. But seriously, i don't understand the problem here. We have so much other, more urgent, stuff to do. |
|
@fuchsto I think we should merge this PR as is. Can you accept |
bug fix for issue #631